Skip to content

fix(files): serve the Claude scratch fallback only from what was opened in its folder (#2259) - #2265

Merged
devswha merged 6 commits into
mainfrom
fix/folder-open-then-check
Oct 28, 2026
Merged

devswha merged 6 commits into
mainfrom
fix/folder-open-then-check

Conversation

@devswha

@devswha devswha commented Oct 28, 2026 •

Copy link
Copy Markdown
Owner

Closes #2259

Problem

The Claude Code scratch-folder fallback of /api/fs/stat and /api/fs/file (findFile → locateFileInFolder) checked the folder's containment on a path, and fileResponse then followed that path again (Bun.file(info.path)). A symlink put in place of the file between the check and the open served a file outside the folder. The file viewer made the window wider still: it asks /api/fs/stat by the name, then fetches /api/fs/file by the absolute path that answered, which matched exactly and was served unconfined.

Change

Built on #2256 (file reads on bounded file threads). Open first, then check what was opened, with the helper rendered pages already use for their own files (openAuthorized: O_NOFOLLOW | O_NONBLOCK, then the opened descriptor's kernel path from /proc/self/fd/N on Linux, owned non-group/other-writable folders on macOS), on the file thread:

  • locateFileInFolder describes a single file (size, modification time, text/binary sniff) on a file thread (describe task, describeWithin) from a descriptor shown to lie in the folder, and returns that folder as within.
  • fileResponse(..., watched, within): the open task opens such a file through openAuthorized and every byte comes from that descriptor. A small body is read before the headers as on fix(files): read the disk for the file routes on bounded background threads (#790) #2256. A larger one is never sent as Bun.file(path) (the "fast disk" branch would follow the path again): on Linux with a fast disk it is sent as a file body named by the descriptor's own path, Bun.file("/proc/self/fd/N").slice(...), which reopens the very file that descriptor holds whatever the path names now, so the answer keeps its exact Content-Length / Content-Range; Bun opens that path only when it sends the answer, so the descriptor is released by a FinalizationRegistry once both the body and the answer are collected, and past 256 such bodies waiting for collection one is streamed from its descriptor instead, so held descriptors stay bounded. On a slow disk, and on macOS and Windows (no /proc), it is streamed from the descriptor (chunked), as fix(files): read the disk for the file routes on bounded background threads (#790) #2256 streams every slow file. The descriptor is named in the pool's cell the moment it is open, so a thread ended mid-check leaves none open (abandon), and a refused one is taken back from the cell before it is closed.
  • A page (render=1) found there is drawn only when the page it opened lies in the folder (render task → pageWithLocalFiles(path, media, grant, within)).
  • findFile takes a mayBeScratch predicate: a path that lies in the pane's scratch folder (the viewer's second request, and a choice among candidates) is found only there, exact and then in another Unicode form, as the scratch fallback finds it, and never by a lookup that is not held to the folder. A path given in full is looked for only where it points (a missing one stays not found, as anywhere else); only a bare name is also found deeper in the folder. file-routes.ts builds it from claudeTempBases() (new in claude-store.ts), so herdr is asked for the pane's session only for a path that can be in one.
  • rootOf/openAuthorized/openedWithin take a hidden flag (default false, the old behaviour for page assets) so the fallback keeps allowing hidden names below the folder as before.

Direct file links outside the scratch folder keep opening any readable file (existing contract). One behaviour change inside the folder: a symlink in the scratch folder that leads out of it no longer opens by its full path either (it already did not by its relative name).

Verification

Rebased onto main after #2256 (d75d738); git range-diff showed the first four commits unchanged from the ones reviewed on #2256's head, and main changed none of the files they touch since. The last two commits answer the merge pass's review of cd6cb60 (M1: a large scratch file lost its Content-Length; M2: a missing full path matched a deeper file).

  • Regression tests (server/file-view.test.ts, "Claude scratch fallback served from what it opened in its folder (fix(files): a symlink swapped in after the scratch-folder containment check serves a file outside the folder #2259)", and server/file-pool.test.ts), deterministic: swaps happen directly or through pageFileHooks hooks between the check and the open, the held thread waits on atomics and a FIFO, no sleeps. Each fails without its fix:
    • after the file was found / between the check and the open / describing / a rendered page / the viewer's stat-then-absolute-path request: fail on fix(files): read the disk for the file routes on bounded background threads (#790) #2256 (the swap serves OUTSIDE-SECRET, the page <p>OUTSIDE-SECRET</p>);
    • "keeps sending the file it opened when a link is swapped in while it is sent" (a small and a 300 KiB body): fails on fix(files): read the disk for the file routes on bounded background threads (#790) #2256, whose fast-disk branch sends the large body as Bun.file(path);
    • "finds a path in the folder only there, in any name form" (NFD→NFC and NFC→NFD swaps): fails before 1947dd7;
    • "sends a file larger than one read with its exact length, whole and by range" (over HTTP): fails on cd6cb60 (no Content-Length); "finds a path given in full in the folder only where it points": fails on cd6cb60 (served <scratch>/sub/a.txt for a missing <scratch>/a.txt);
    • the cell tests for open and describe, and the pool test that ends a thread held inside the check: fail when the cell take-back or abandon's describe case is removed (mutation checks); the pool test passes 50 runs pinned to one core.
  • bun test server/file-view.test.ts server/file-routes.test.ts server/claude-store.test.ts server/file-pool.test.ts: 185 pass, 0 fail. bun run typecheck clean. bun run check fast: green on e0da0aa (unit 8225 pass, 0 fail; typecheck, build, generated types and themes, workflow syntax ok).
  • HTTP, on a Bun.serve running the route's own steps with the real file-thread pool against a temp scratch folder under <tmp>/claude-<uid>/…: stat by name 200 → file by the returned absolute path 200; Range: bytes=2-4 206 bytes 2-4/10; HEAD content-length: 10; a 300 KiB body arrives whole from the descriptor; render=1 page 200. After swapping the three files for links outside: file, stat, the large body and the page all 404; a direct link to the outside file still 200 (contract).
  • No REMOTE_BUNDLE_VERSION change: only a release PR raises it.

Reviews

omo-native-code-reviewer and Codex Sol 6.1 Fast xhigh, on the first version and again on the port onto #2256; every HIGH and MEDIUM fixed, final rounds PASS from both. Findings and resolutions are in the PR comment.

Known limits (LOW, not fixed here)

  • Candidate flake in fix(files): read the disk for the file routes on bounded background threads (#790) #2256's own test "keeps a descriptor its thread is reading open until that thread is gone, when the last server stops": its fake thread (readGate) says it is reading before it opens the FIFO, and the test then opens the FIFO's writer blocking, so a thread ended in between hangs the test (Bun's timeout cannot fire while the main thread is blocked). Codex reproduced that gap with taskset -c 0 in this PR's first version of the new pool test; this PR's authorizeGate opens the FIFO read-write before it says it ran and is released by a non-blocking write, and the same change would fit readGate.
  • On a slow disk (or macOS/Windows), a scratch file larger than one read (256 KiB) is streamed from its descriptor without a Content-Length, as fix(files): read the disk for the file routes on bounded background threads (#790) #2256 streams every slow file; a fast disk on Linux keeps it.
  • On Linux, the reopen through /proc/self/fd/N checks permissions again: a scratch file made unreadable (chmod 000) after it was opened answers 500, as does a Linux system without /proc. Both fail closed (no other bytes are sent).
  • When herdr cannot name the pane's scratch folder at the time of a request (Claude exited, herdr busy, a new session), a full path into it is a direct link like any other absolute path.
  • A scratch file whose name starts with ~ or has leading/trailing spaces is not found by its full path (the fallback's own name rules).
  • A hardlink placed in the scratch folder to a file outside it is still served: a descriptor cannot tell a hardlink, the page-asset path has the same property, and making one needs write access to the folder and to the target's filesystem.
  • A scratch file whose name ends in (deleted), and on macOS a scratch subfolder other users can write to, are now refused by the fallback (same rules as page assets).

@devswha

devswha commented Oct 28, 2026

Copy link
Copy Markdown
Owner Author

Review findings and how each was resolved (this PR touches an HTTP contract and file-access confinement, so the code-review agents' reviews are recorded here; CodeRabbit's will be added if it runs).

omo-native-code-reviewer on 346a36c: FAIL

  • HIGH H1: the file viewer stats by name, then fetches by the absolute path stat returned (src/components/FileViewer.tsx 212/226/326, src/lib/markdownImages.ts 49), which matched exactly in findFile and was served through Bun.file; a link swapped in between served the outside file. Fixed in 5cc3aa9: an exact path in the pane's scratch folder goes through locateFileInFolder and is served from the descriptor opened there; regression test fails on 346a36c, passes on 5cc3aa9; HTTP reproduction now 404.
  • LOW L1: candidates were checked by path only. Covered by the same fix: the client opens a chosen candidate by its absolute path, which is now confined.
  • LOW L2: a hardlink in the folder is served. Not fixed: no descriptor check can tell it, same as page assets; noted in the PR body.
  • LOW L3: (deleted) names and group-writable macOS scratch subfolders are refused now. Accepted, same rules as page assets; noted in the PR body.
  • LOW L4: a second fstat and a fd! in kindOf. Left: the second fstat gives the Date form FileInfo.modified needs (the authorized record holds nanoseconds only), and the assertion is guarded by the branch above it.

Codex Sol 6.1 Fast xhigh

  • 346a36c: PASS. LOW: no test that a swap after authorization keeps the opened file. Added ("keeps sending the file it opened…", including download=1); mutation check (body from Bun.file(info.path)) makes it fail.
  • Delta 346a36c..5cc3aa9: PASS, no findings (prefix checks keep the same lexical spelling across both requests, including symlinked temp roots).
  • Note: Codex's first attempt on 346a36c was stopped by its provider's content filter; the rerun with a reworded brief completed.

@coderabbitai

coderabbitai Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 5cc3aa9 and e0da0aa.

Files selected for processing: 7
  • server/file-pool.test.ts
  • server/file-pool.ts
  • server/file-routes.ts
  • server/file-tasks.ts
  • server/file-view.test.ts
  • server/file-view.ts
  • server/file-worker.fake.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

📝 Summary

Summary by CodeRabbit

  • Bug Fixes
    • Files opened from a Claude Code session’s scratch folder now continue to display and download the file opened there, even if its link is later replaced with a link to a file outside the folder.
    • Name-based file lookups and rendered pages stay within the scratch folder.
    • Preserved whole-file, ranged, and HEAD responses for scratch-folder files.

Claude scratch-folder lookup now carries its folder boundary into file serving. The server authorizes opened files within that boundary and uses the authorized descriptor for responses. Tests cover replaced links, response modes, and descriptor cleanup.

Changes: Claude scratch-folder file containment

Layer / File(s) Summary
Identify scratch-folder matches
server/claude-store.ts, server/file-view.ts, server/file-routes.ts, server/file-view.test.ts
Shared Claude temporary roots identify possible scratch paths. Lookup returns matching files with their folder boundary, and the route passes that boundary to file serving.
Authorize and serve files within the folder
server/file-view.ts, server/file-tasks.ts, server/file-view.test.ts, changelog.d/scratch-folder-open-then-check.md
Constrained file tasks authorize opened descriptors within the folder. Rendering and responses receive the folder boundary. Fast responses use the authorized descriptor when available. Tests cover replacement links, content, and whole-file, range, and HEAD responses.
Track and release opened descriptors
server/file-tasks.ts, server/file-pool.ts, server/file-worker.fake.ts, server/file-pool.test.ts, server/file-view.test.ts
Describe and open tasks record opened descriptors in shared cells. Abandoned tasks close recorded descriptors. Tests check descriptor cleanup after worker replacement and during authorization.

Priority: Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Possibly related issues

  • devswha/ovis#2259: Describes the scratch-folder containment race addressed by authorizing and serving the opened file within the folder.

Possibly related PRs

  • devswha/ovis#1980: Changes the lookup contract that this PR extends with folder-bound results.
  • devswha/ovis#1696: Adds the file-route flow that this PR updates to pass scratch-folder predicates and folder boundaries.
  • devswha/ovis#1927: Covers descriptor-bound serving for rendered pages, which this PR also applies to scratch-folder fallback files.

Suggested reviewers: jensr93

Merge Risk: Minimal · up to e0da0

No actionable issue was established in the scratch-folder containment change; it is mergeable after normal checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 76.47% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the primary change: limiting Claude scratch-folder fallback responses to the file opened within that folder.
Description check Passed The description explains the problem, implementation, scope, known limits, linked issue, and detailed validation results. It uses "## Verification" instead of the template's "## Validation" heading, b…
Linked Issues check Passed [#2259] requires the Claude scratch-folder fallback to open the file, authorize the opened descriptor, and serve only from that descriptor. Verified at head: openAuthorized (server/file-view.ts) now…
Out of Scope Changes check Passed Every changed area maps to #2259. openAuthorized/openedWithin/rootOf gain hidden, cell, and exported types so the fallback can use the page-asset check. describe and open/render file t…
Full details: Description check

Explanation

The description explains the problem, implementation, scope, known limits, linked issue, and detailed validation results. It uses "## Verification" instead of the template's "## Validation" heading, but it provides the required validation information and is otherwise complete.

Full details: Linked Issues check

Explanation

[#2259] requires the Claude scratch-folder fallback to open the file, authorize the opened descriptor, and serve only from that descriptor. Verified at head: openAuthorized (server/file-view.ts) now takes hidden and cell parameters. It opens the file, then proves the descriptor lies under the root (/proc/self/fd/N on Linux, owned-folder check on macOS). describeWithin derives size, mtime, and text/binary sniff from that descriptor (kindOf reads opened). locateFileInFolder now returns a describe result with within, and fileResponse takes within and opens through openAuthorized. The PR reports that the small body is read from the descriptor, the large Linux fast-disk body is sent from /proc/self/fd/N (same inode as the authorized descriptor), and other bodies stream from the descriptor. Rendered pages are confined by pageWithLocalFiles(..., within), which checks openedWithin on the page descriptor. The viewer's exact-absolute-path request is confined by findFile with mayBeScratch. Regression tests are reported as failing on #2256 and passing here. Remaining uncertainty: the fileResponse body of the diff was truncated in my read, so the fast-disk branch was checked only through the PR description and summary, not the final code lines. The previous PASS is still supported by the changed code.

Full details: Out of Scope Changes check

Explanation

Every changed area maps to #2259. openAuthorized/openedWithin/rootOf gain hidden, cell, and exported types so the fallback can use the page-asset check. describe and open/render file tasks in server/file-tasks.ts and server/file-pool.ts (cell, abandon) carry the descriptor from the thread to the caller, which is needed for mid-check cleanup. findFile's mayBeScratch and claudeTempBases (server/claude-store.ts, used by server/file-routes.ts) keep the viewer's second request, which uses an exact absolute path, inside the same folder confinement. The behavior changes (a scratch symlink leading out is refused by its full path; a (deleted) name and a non-hidden scratch subfolder are refused) follow the fallback's existing rules. The fake-worker and FIFO test changes (server/file-worker.fake.ts, server/file-pool.test.ts) support the regression tests for these paths. The changelog entry documents the fix. Direct links outside the scratch folder keep their existing behavior, so the PR does not widen unrelated behavior.

Full details: Docstring Coverage

Explanation

Docstring coverage is 76.47% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 17 functions across 8 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

…ed in its folder (#2259)

locateFileInFolder checked the scratch folder's containment on a path, and the
file thread's open then followed that path again (and a large body was sent
from the path by Bun.file), so a symlink put in place of the file in between
served a file outside the folder. The viewer widened the window: it stats by
name, then fetches by the absolute path that answered, which matched exactly
and was served unconfined.

Open first, then check what was opened (openAuthorized: O_NOFOLLOW, then the
descriptor's kernel path on Linux, owned folders on macOS), on the file thread:
- the 'describe' task describes a fallback file from that descriptor and returns
  the folder as 'within';
- the 'open' task, given 'within', opens through openAuthorized (naming the
  descriptor in the pool's cell at once, and taking a refused one back before
  closing it) and never sends the body from the path;
- the 'render' task draws a page found there only when the page it opened lies
  in the folder;
- findFile confines an exact path that lies in the pane's scratch folder
  (claudeTempBases tells such a path before herdr is asked).
Direct file links outside the scratch folder keep opening any readable file.
… form (#2259)

A path in the pane's scratch folder with no exact match fell through to the
any-form lookup that is not held to the folder, so a link put there under the
other Unicode form of the file's name (NFD for NFC) was served from outside
(omo review MEDIUM on the port). Such a path is now found only through
locateFileInFolder, exact and then in any form, and never past it; the
scratch folder is asked for once per lookup. describeWithin no longer opens
the path again through fileInfo: the type comes from the name, the rest from
the descriptor.
…read is ended (#2259)

The describe task opened the scratch file through openAuthorized without the
pool's cell, so a thread ended inside the check left it open (Codex review
MEDIUM). describe now names its descriptor in a cell as open does, takes it
back before closing it, and abandon closes it once an ended thread is gone.
The name-form test covers both swap directions (NFD for NFC and back).
… waits (#2259)

The fake thread said it ran before it opened the FIFO, so a test that ended
it in between then blocked forever opening the FIFO's writer with no reader
(Codex review MEDIUM, reproduced on one core). The thread now opens the FIFO
read and write before it says it ran and blocks in a read; the test releases
it with a non-blocking write.
@devswha
devswha force-pushed the fix/folder-open-then-check branch from 5cc3aa9 to cd6cb60 Compare October 28, 2026 14:18
@devswha

devswha commented Oct 28, 2026

Copy link
Copy Markdown
Owner Author

Rebased onto main after #2256 (d75d738) and ported into the file threads; head cd6cb60. Review rounds since the first comment (omo-native-code-reviewer and Codex Sol 6.1 Fast xhigh):

After rebasing onto #2256 (file-thread pool): the fix was ported into the file thread's open, render and new describe tasks, and both reviewers ran again on the port.

  • omo on the port (5eaf532b): MEDIUM, a path in the scratch folder with no exact match fell through to the any-Unicode-form lookup that was not held to the folder (a link under the other NFC/NFD form of the name served the outside file). Codex rated the same finding HIGH. Fixed (findFile finds such a path only in the folder, never past it); the regression test covers both swap directions and fails before the fix.
  • Codex on the port: MEDIUM, the describe task opened without the pool's cell, so a thread ended mid-check leaked its descriptor. Fixed (cell + abandon for describe); new file-pool test with a thread held inside the check, which fails when abandon skips describe.
  • omo LOWs on the port, not fixed: a scratch file whose name starts with ~ or has leading/trailing spaces is not found by its full path (the fallback's own name rules); when herdr cannot name the pane's scratch folder at the time of the GET (Claude exited, herdr busy), the path is treated as a direct link, as any other absolute path; each scratch-file request adds a herdr/transcript lookup; the real mayBeClaudeTemp predicate is not unit-tested (exercised by the HTTP run). Fixed: describeWithin no longer opens the path again through fileInfo.
  • Codex on the fixes (241d1fb4): both production findings confirmed fixed; MEDIUM, the new pool test could deadlock (the fake thread said it ran before it opened the FIFO, and the test's blocking writer open then waited for a reader that never came; reproduced with taskset -c 0). Fixed in e23ff3c2: the thread opens the FIFO read-write before it says it ran and blocks in a read, and the test releases it with a non-blocking write; 50 runs on one core pass, and the test still fails when abandon skips describe.
  • omo on the fixes (241d1fb4): PASS (re-ran its own NFD/NFC reproduction: now refused).

@devswha

devswha commented Oct 28, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@devswha

devswha commented Oct 28, 2026

Copy link
Copy Markdown
Owner Author

CodeRabbit did not review head cd6cb60 (its "@coderabbitai review" answered "Review rate limited"). The code-review agents' reviews stand in: omo-native-code-reviewer and Codex Sol 6.1 Fast xhigh, final rounds PASS on these exact four commits (git range-diff against the reviewed ones shows them identical); their findings and how each was resolved are in the two comments above.

@coderabbitai

coderabbitai Bot commented Oct 28, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

@devswha Understood. CodeRabbit did not review head cd6cb60e; the review request was rate limited. Your comments document the other reviewers’ final PASS results and the reported commit equivalence. This reply does not constitute an independent review or approval.

You are interacting with an AI system.

… found (#2259)

Two regressions the merge pass's review found in this PR:
- A scratch file larger than one read was streamed from its descriptor
  without a Content-Length (Bun sends any stream chunked), so a download
  showed no size. On Linux a fast disk now sends it as a file body named by
  the descriptor's own path (/proc/self/fd/N, the file that descriptor holds,
  whatever its path names now), which Bun sizes and slices exactly. Bun opens
  that path only when it sends the answer, so the descriptor is closed once
  both the body and its answer are collected. A slow disk, and macOS and
  Windows, still stream from the descriptor.
- A full path into the scratch folder that is not there was matched against
  files deeper in the folder whose path ends in it. Like a full path anywhere
  else, it is now looked for only where it points (in either Unicode form);
  a bare name is still found deeper.
…ected (#2259)

Each fast scratch body larger than one read holds its descriptor until the
body and its answer are collected, so a player seeking through a long video
could pile them up between collections (Codex review LOW). Past 256 such
bodies waiting, one is streamed from its descriptor instead. The range test
now asks for more than one read with bytes that tell their offset, so it
exercises the file body rather than the read before the headers.
@devswha

devswha commented Oct 28, 2026

Copy link
Copy Markdown
Owner Author

Fix round for the merge pass's review of cd6cb60 (two MEDIUMs), now at e0da0aa:

  • M1, a scratch file over 256 KiB lost its Content-Length (streamed chunked). Bun sends any stream body chunked and ignores an explicit Content-Length on it (measured), and Bun.file(fd) was unusable (its slices hang or send a wrong length, and Bun never closes the fd). Fixed in 3b5ee9d: on Linux with a fast disk the answer is Bun.file("/proc/self/fd/N").slice(...), which reopens the very file the checked descriptor holds whatever the path names now, with an exact Content-Length/Content-Range; the descriptor is released once the body and its answer are collected (Bun opens that path only when it sends the answer). Regression test over HTTP (whole and a range larger than one read, bytes that tell their offset) fails on cd6cb60; an offset mutation fails it too.
  • M2, a missing full path matched a deeper file (<scratch>/a.txt served <scratch>/sub/a.txt). Fixed in 3b5ee9d: a path given in full is looked for only where it points (either Unicode form); a bare name still walks. Regression test fails on cd6cb60.

Reviews of this round:

  • Codex Sol 6.1 Fast xhigh (3b5ee9d): PASS. LOWs fixed in e0da0aa: descriptors held until collection are now bounded (past 256 waiting, a body is streamed from its descriptor), and the range test now asks for more than one read so it exercises the file body.
  • omo-native-code-reviewer (3b5ee9d): PASS; it reproduced the worst case (fd closed one turn after the answer and its number reused by a decoy: 200 + 20 slow clients, no wrong body; Bun opens the link when it renders the answer). LOWs: finalizer close hardened and the test helper's deadline set below the test timeout (e0da0aa); a chmod-000-after-open or /proc-less Linux answers 500 (fails closed, noted in the PR body); the non-Linux branch keys on process.platform, not a test hook (left).

@devswha

devswha commented Oct 28, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@devswha

devswha commented Oct 28, 2026

Copy link
Copy Markdown
Owner Author

Merging e0da0aa (closes #2259). File serving and access, so it got a strict review of its own from the merge pass's reviewer (route-coverage QA with a real createServer and the real file pool), on top of the lane's omo and Codex reviews. When a Claude pane names a file in its scratch folder, the server now opens it first and checks what it opened (the descriptor's identity and real path, a regular file inside the folder), then serves that same descriptor, also through #2256's worker pool via openAuthorized. Verified: on main a file swapped for a link before the open, or a linked file or folder, or an NFC/NFD variant, served an outside file; on this head every such case is refused (404), a swap after the check still serves the checked file's bytes (60 forced-GC tries, never the outside file), a FIFO gets 404 without holding a worker, and descriptors do not leak. This round (Dori's call): large scratch files keep an exact Content-Length and Content-Range (307200; a 1000-299999 range gives 206 with 299000 bytes), sent from the checked descriptor; a missing absolute path is not found, while a bare name is still looked up in the folder. Descriptors waiting to be collected stay under the 256 cap plus bodies in flight, and return to zero after a GC. Not filed: up to about 256 descriptors can stay open for seconds to ~45 s after a burst (a deleted scratch file's space stays allocated until then), and near a hard fd limit of about 300 they can make other requests fail; an aborted fast body keeps its descriptor until a GC; the bodiesCollected test helper polls under a 3 s deadline; the fileResponse JSDoc is now separated from its function.

@devswha
devswha merged commit 897ca66 into main Oct 28, 2026
26 of 29 checks passed
@devswha
devswha deleted the fix/folder-open-then-check branch October 28, 2026 16:39
devswha added a commit that referenced this pull request Oct 28, 2026
…ading (#2296) (#2297)

Closes #2296

## Problem

`server/file-pool.test.ts` hung CI's Fast checks on run
[40779586477](https://github.com/devswha/ovis/actions/runs/40779586477)
(#2293, which does not touch it): the job was cancelled at its 10-minute
limit with this file the only one never reported.

The test "keeps a descriptor its thread is reading open until that
thread is gone, when the last server stops" holds its file thread with
the fake worker's `readGate` (`server/file-worker.fake.ts`, from #2256).
That gate said the thread was reading (`Atomics.notify` on cell 1)
**before** it opened the FIFO for reading, a blocking `open(2)`. The
test then ends the thread (`release()`) and opened the FIFO's writer
with a blocking `openSync(fifo, "w")` on its main thread. A thread ended
between the notify and its own open never opens the reader, so the
writer's open waits forever, and Bun's per-test timeout cannot fire
while the main thread is blocked in a system call. #2265's PR body named
this gap as a candidate flake.

## Change (test-only)

- `readGate` opens the FIFO read-write (an open that never waits)
**before** it records the descriptor and notifies, then blocks in a
one-byte read of it, as `authorizeGate` (#2265) already does. Once the
test sees the thread running, the FIFO always has a reader, even after
that thread is ended.
- The test releases the read with `O_WRONLY | O_NONBLOCK` and a one-byte
write, which never waits (and would fail with `ENXIO` rather than hang
if there were no reader).
- Both FIFO gates (`readGate`, `authorizeGate`, now one `holdOnFifo`)
name their FIFO descriptor in a third gate cell, and the test closes it
once the thread is gone: a worker ended inside that read never runs its
`finally` on Bun, so before this each run left 2 FIFO descriptors open
(found from CodeRabbit's review; `/proc/self/fd` count at the end of the
file: 2 -> 0).

`server/file-pool.ts` is not changed: the server's thread never waits on
a file thread (it posts a task, ends a thread with `terminate()` without
awaiting it, and closes descriptors), so the product cannot block this
way. Only the tests await `fileThreadsClosed()`.

## Verification

- Widened window, scratch only (a 500 ms `Atomics.wait` right after the
notify in `readGate`):
- on main 72f2edf: `timeout 30 bun test server/file-pool.test.ts` is
killed at 30 s (exit 124), stopped in that test;
  - with this fix: exit 0 in 1 s, 12 pass.
- 50 runs of `bun test server/file-pool.test.ts`, 10 at a time pinned to
2 cores (`taskset -c 0,1`), each under `timeout 60`: 50 pass, 0 hangs.
Again pinned to one core (`taskset -c 0`): 50 pass, 0 hangs.
- Leaked FIFO descriptors at the end of the file (`/proc/self/fd`,
scratch probe): 2 before, 0 after; on 80b8cb2 the widened window again
passes in 1 s and 50/50 runs pinned to one core pass.
- `bun run check fast`: green on 80b8cb2 (typecheck, build, generated
files, workflow syntax; unit 8380 pass, 0 fail, 34 skip in 535 files).

## Review

omo-native-code-reviewer: PASS (APPROVE), no CRITICAL/HIGH/MEDIUM. It
found no path left that blocks the test's main thread (the remaining
waits are the non-blocking open and write, `hold.ran()`'s 10 s bounded
wait, and awaited promises), confirmed the release cannot hit
ENXIO/EAGAIN, and checked that a failing assertion while the thread is
parked fails fast instead of hanging. A second omo round on 80b8cb2
(the FIFO-descriptor take-back): PASS (APPROVE), reproduced 2 leaked
FIFO descriptors at 65a8a71 and none at 80b8cb2. No Codex review:
test-only, `server/file-pool.ts` unchanged.

## Known limits (LOW, not fixed here)

- If the thread is ended after its notify but before it enters the FIFO
read, the test still passes but does not exercise a read in flight in
that run (a false pass, never a hang). Every gate that notifies before
it blocks has this window, `authorizeGate` included.


The reviewer's findings and how each was resolved are in a PR comment.
devswha added a commit that referenced this pull request Oct 29, 2026
…ollected (#2286) (#2317)

Closes #2286

## Problem

A Claude Code scratch file larger than one read (256 KiB) on a fast
Linux disk is sent as `Bun.file("/proc/self/fd/N").slice(...)` (#2265),
a file body Bun sizes itself. Its descriptor must outlive Bun's lazy
open, so a `FinalizationRegistry` closes it once both the body and its
answer are collected. Past 256 such bodies waiting for collection,
`fileResponse` streamed the body from its descriptor instead, and Bun
drops the Content-Length of any stream body. Many parallel downloads
reach that cap before a collection runs. Reproduced on main with 300 KiB
scratch files, 1,800 requests over 16 parallel connections from another
process: **73 to 98 of 1,800** answers came back chunked, with no size
or progress.

## Change

- **Cap:** raised from 256 to a quarter of this process's open-file
limit, at most 32,768 descriptor-named bodies.
- Only bodies actually being sent are really held, and those are bounded
by open connections. The rest are sent bodies waiting for a collection.
- Sent bodies that lived through a collection while they were sent wait
for one of Bun's own full collections, so the held count rises and falls
with those. The peaks, measured with no cap on a 1M-object heap, were
1,942 over 60,000 downloads 16 at a time and 6,657 over 30,000 downloads
64 at a time.
- The limit is read once from `/proc/self/limits`; only Linux takes this
path. Bun raises the soft limit to the hard one at start (a soft limit
of 1,024 became 524,288 here), so most hosts get 32,768.
- A host whose hard limit is 1,024 keeps main's 256, leaving the rest
for the server's own sockets, terminals and file threads. When the limit
cannot be read the cap is 256.
- **Collection requested, never forced:** past 256 held bodies (a
quarter of the cap when that is lower), a collection is requested on the
collector's own threads (`Bun.gc(false)`), at most once every 250 ms,
and no answer waits for it.
- That frees only bodies made since the last collection: about half the
peak in a burst, not ones that lived through a collection.
- There is never a forced collection (`Bun.gc(true)`), which holds every
terminal, key and request on the server's thread for as long as it takes
on the whole heap.
- Past the cap, a body is still streamed from its descriptor, as before.
- **Unchanged:** descriptor handling and #2259's confinement. A
descriptor is still closed only once both the body and its answer are
collected.

The first version of this PR forced a collection at the cap. The merge
pass measured it stopping the event loop for 215–300 ms per collection
on a ~300 MB heap during a seek burst, so it was replaced.

Alternatives measured on Bun 1.4.3 and rejected:
- **A sized `ReadableStream`:** Bun drops Content-Length for every
stream body.
- **`Bun.file(fd).slice()`:** Bun sends the whole file's size as
Content-Length with only the slice's bytes, a wrong header.
- **Closing the descriptor when the body is done:** there is no hook for
it, because `request.signal` does not abort after a sent or a cut
answer.
- **Requesting a collection early while keeping the 256 cap:** 88–97 of
1,800 were still chunked, because the collection lands too late.
- **The same, plus a non-blocking wait at the cap:** 52–64 of 1,800
chunked, about 2.5 times slower.

## Verification

- **Load test** (separate client process, the real file-thread pool, 300
KiB scratch files, 16 at a time):
  - main: 73–98 of 1,800 chunked in every run.
- This branch: 0 of 1,800 in every run, and 0 of 10,000 on a 1M-object
heap. Every body had the right bytes and length.
- **Sustained load** (64 at a time, 30,000 downloads, 1M-object heap): 0
of 30,000 chunked, where a 4,096 ceiling lost 3,166 to 8,086 and main
lost 28,976.
- **Seek burst** (1M-object heap, 256 bodies truly held, 512 range
requests 16 at a time, the server's event-loop lag sampled every 2 ms),
three alternating rounds:

| Tree | Stalls over 50 ms per burst | Answers without a length | Burst
time |
|---|---|---|---|
| main | 1 (99–191 ms) | 512 of 512 | 254–373 ms |
| first version (0f2a44a) | 9 (786–860 ms in total) | 512 of 512 |
844–1,161 ms |
| this branch | 1 (132–184 ms, the heap's own collection, as on main) |
0 of 512 | 265–478 ms |

- **New tests** in `server/file-view.test.ts`, neither using sleeps:
- "keeps a large file's exact length while many answers let go of wait
to be collected (#2286)": it lets go of 1,024 answers, makes the next
one before the event loop turns, and fetches it over HTTP. It fails 3 of
3 on main (`content-length` null) and passes here.
- "holds no more such bodies than a quarter of this process's open files
(#2286)": with the limit set to 1,024, the answer after 256 let-go
answers is streamed, whole. It fails 3 of 3 on the round-2 version
(6eda965), which ignored the limit, and passes here.
- "answers a burst of ranges while many bodies are held without stopping
the event loop for a collection (#2286)": the seek-burst bound. It wraps
`Bun.gc` and asserts that no forced collection runs while 1,024 ranges
are held. It fails 3 of 3 on the first version and passes here.
- The bound is not a wall-clock event-loop limit, which would flake on
loaded runners, as this repo's timing ratios have (#2084, #1901).
- **A host whose hard limit is 1,024** (Codex's reproduction: `ulimit
-Sn 1024; ulimit -Hn 1024`, 256 reserved descriptors, 10,000 range
reads, a 1M-object heap): all 10,000 answered 206 with the right bytes,
peaking at 542 descriptors, the same as main's 542. The round-2 version
returned 7,968 404s with `EMFILE`. Each collection request cost 1.5–5.9
ms on the thread.
- `bun test server/file-view.test.ts server/file-pool.test.ts
server/file-routes.test.ts server/claude-store.test.ts`: 197 pass, 0
fail. `bun run typecheck` is clean.
- No `REMOTE_BUNDLE_VERSION` change: only a release PR raises it.

## Reviews

The round-1 reviews (omo, Codex Sol 6.1 Fast xhigh) and the merge pass's
MEDIUM are in the PR comments. The round-2 reviews of this design are in
a comment below.

## Known limits (LOW, not fixed here)

- **Slow disks and other platforms still stream without a length.** On a
slow disk, a scratch file larger than one read is still streamed without
a Content-Length, and so is every such file on macOS and Windows, as
before (#2265).
- **Past the cap, answers stream without a length again.** Under
sustained load with no full collection the held count keeps growing
(about 270 a second at 64-way in omo's 96 s run, 25,626 held after
150,000 downloads), so the 32,768 cap is reached after roughly 200,000
downloads at that rate, about 800 MB/s for minutes. The answers past it
still arrive whole.
- **A larger cap keeps deleted scratch files' disk space a while
longer.** Up to 32,768 descriptors, against 256 before, can keep a
deleted file's space in use until a full collection. On a host whose
hard limit is 1,024, the cap stays 256, so heavy parallel load there
loses lengths as on main.
- **Held descriptors wait for a collection.** Up to a quarter of the
open-file limit (at most 32,768) can stay open until a collection runs,
where up to 256 could before.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(files): a symlink swapped in after the scratch-folder containment check serves a file outside the folder

1 participant